Skip to content

chore: rename timestampSeconds to timestampMs in ActionsService#246

Merged
gjtorikian merged 1 commit intomainfrom
chore/rename-actions-timestamp-var
Apr 30, 2026
Merged

chore: rename timestampSeconds to timestampMs in ActionsService#246
gjtorikian merged 1 commit intomainfrom
chore/rename-actions-timestamp-var

Conversation

@gjtorikian
Copy link
Copy Markdown
Contributor

Summary

  • Pure rename of a misleading local variable name in ActionsService.VerifyHeader. No behavior change.
  • The variable was named timestampSeconds, but it holds the raw t= value parsed from the signature header — which WorkOS sends as a millisecond Unix timestamp — and is compared against DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(). So the math was already correct in milliseconds; only the name was wrong.
  • Caught while auditing other backend SDKs after the recent Go SDK fix that addressed the same millisecond-vs-seconds confusion at the parsing layer (workos-go f96ac76). The other SDKs (elixir, kotlin, php, python, ruby, dotnet) all correctly treat t= as ms; the dotnet variable name was the only lingering rough edge.

Test plan

  • CI passes (no test changes — this is a rename within a method body)
  • No callers reference the variable (it's a local)

🤖 Generated with Claude Code

The local variable was named `timestampSeconds` but holds a millisecond
Unix timestamp (parsed from the `t=` value, which WorkOS sends in ms)
and is compared against `DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()`.
The math was correct; only the name was misleading.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@gjtorikian gjtorikian requested review from a team as code owners April 30, 2026 16:45
@gjtorikian gjtorikian requested a review from marji-workos April 30, 2026 16:45
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR renames the local variable timestampSeconds to timestampMs in ActionsService.VerifyHeader to accurately reflect that the t= field in the WorkOS signature header carries a millisecond Unix timestamp. The tolerance math (tolerance * 1000L) was already correct; only the name was misleading.

Confidence Score: 5/5

Safe to merge — purely a clarifying rename with no behavior change.

Single local variable rename confirmed correct: timestampMs accurately names the millisecond value, and tolerance * 1000L already converts the seconds-based tolerance to milliseconds. No logic, signature computation, or external contract is affected.

No files require special attention.

Important Files Changed

Filename Overview
src/WorkOS.net/Services/Actions/ActionsService.cs Pure local variable rename from timestampSeconds to timestampMs in VerifyHeader; no logic changes, no behavior impact.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant VerifyHeader
    participant ParseSignatureHeader
    participant ComputeSignature

    Caller->>VerifyHeader: payload, sigHeader, secret, tolerance
    VerifyHeader->>ParseSignatureHeader: sigHeader
    ParseSignatureHeader-->>VerifyHeader: (timestamp string, signature string)
    VerifyHeader->>VerifyHeader: timestampMs = long.Parse(timestamp)
    VerifyHeader->>VerifyHeader: nowMs = UtcNow.ToUnixTimeMilliseconds()
    VerifyHeader->>VerifyHeader: check |nowMs - timestampMs| > tolerance * 1000
    VerifyHeader->>ComputeSignature: timestamp, payload, secret
    ComputeSignature-->>VerifyHeader: expectedSignature (HMAC-SHA256)
    VerifyHeader->>VerifyHeader: SecureCompare(expectedSignature, signature)
    VerifyHeader-->>Caller: OK or throws InvalidOperationException
Loading

Reviews (1): Last reviewed commit: "chore: rename timestampSeconds to timest..." | Re-trigger Greptile

@gjtorikian gjtorikian merged commit 571346f into main Apr 30, 2026
7 checks passed
@gjtorikian gjtorikian deleted the chore/rename-actions-timestamp-var branch April 30, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant